fix(taskfiles): Improve concurrency control for external tools:#117
Merged
Conversation
… set by a user for concurrency related variables; Tasks that use the concurrency value default to nprocs.
Contributor
WalkthroughThis PR adds a shared ChangesJOBS Concurrency Defaulting
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
exports/taskfiles/utils/boost.yaml (1)
154-160: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep a local
JOBSdefault here.download-and-installnow passes{{.JOBS}}straight through tobuild-and-install; if it’s unset, Task will expand that to<no value>, which prevents the child task’sdefault .G_NPROCS .JOBSfallback from doing its job and can surface as an invalid-jvalue on the default path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@exports/taskfiles/utils/boost.yaml` around lines 154 - 160, The boost task variables block is missing a local JOBS default, so when download-and-install forwards {{.JOBS}} into build-and-install it can expand to <no value> and bypass the child task’s fallback. Update the variables section in boost.yaml near BUILD_DIR and BUILD_AND_INSTALL_ARGS to define JOBS with a default that falls back to .G_NPROCS when .JOBS is unset, so build-and-install can safely receive a valid jobs value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@exports/taskfiles/utils/boost.yaml`:
- Around line 154-160: The boost task variables block is missing a local JOBS
default, so when download-and-install forwards {{.JOBS}} into build-and-install
it can expand to <no value> and bypass the child task’s fallback. Update the
variables section in boost.yaml near BUILD_DIR and BUILD_AND_INSTALL_ARGS to
define JOBS with a default that falls back to .G_NPROCS when .JOBS is unset, so
build-and-install can safely receive a valid jobs value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 22618304-b326-4ec4-9998-487ac7d8c79e
📒 Files selected for processing (3)
exports/taskfiles/utils/boost.yamlexports/taskfiles/utils/cmake.yamlexports/taskfiles/utils/misc.yaml
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
exports/taskfiles/utils/cmake.yaml (1)
172-172: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd a
CMAKE_JOBSvar ininstall-remote-tar.{{.CMAKE_JOBS}}is unset here, sobuildgets an emptyJOBSvalue and falls back to its own default concurrency instead of the caller’s setting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@exports/taskfiles/utils/cmake.yaml` at line 172, The install-remote-tar task is missing the caller-controlled CMAKE_JOBS value, so the build step can ignore the intended concurrency setting. Update the install-remote-tar task in the cmake.yaml template to pass through .CMAKE_JOBS alongside the existing install args, and ensure the build invocation uses that variable instead of leaving JOBS empty. Use install-remote-tar and the CMAKE_JOBS/CMAKE_INSTALL_ARGS symbols to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@exports/taskfiles/utils/cmake.yaml`:
- Line 172: The install-remote-tar task is missing the caller-controlled
CMAKE_JOBS value, so the build step can ignore the intended concurrency setting.
Update the install-remote-tar task in the cmake.yaml template to pass through
.CMAKE_JOBS alongside the existing install args, and ensure the build invocation
uses that variable instead of leaving JOBS empty. Use install-remote-tar and the
CMAKE_JOBS/CMAKE_INSTALL_ARGS symbols to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 51d9e739-1dc1-4704-bff2-3c77d6ce8762
📒 Files selected for processing (2)
exports/taskfiles/utils/boost.yamlexports/taskfiles/utils/cmake.yaml
davidlion
changed the title
TASK_EXTERNAL_TOOL_CONCURRENCY env variable, with nprocs as a fallback.
kirkrodrigues
approved these changes
Jul 9, 2026
kirkrodrigues
left a comment
kirkrodrigues
left a comment
Member
There was a problem hiding this comment.
For the PR title & body, how about:
fix(taskfiles): Improve concurrency control for external tools:
- Default to the number of logical processors.
- Allow users to control the default through the env var `TASK_EXTERNAL_TOOL_CONCURRENCY`.
TASK_EXTERNAL_TOOL_CONCURRENCY env variable, with nprocs as a fallback.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TASK_EXTERNAL_TOOL_CONCURRENCY.Description
Currently, concurrency variables are not handled consistently so a user cannot easily control the concurrency from the command line. The goal of this PR is to create a consistent flow for users to easily control concurrency in resource limited scenarios.
For example, most tasks use the variable
JOBS, but cmake also has a task that usesCMAKE_JOBSand passes that value as aJOBSargument. If the user setsJOBSit will not be respected by the task usingCMAKE_JOBS. This is further complicated as top-level projects (e.g. CLP) end up using their own ad-hoc variables to control concurrency.Tasks that set a concurrency related parameter for a tool should (see PR code for example):
TASK_EXTERNAL_TOOL_CONCURRENCY(if non-empty).G_NPROCSin misc.yaml.Tasks that propagate a concurrency related parameter should:
FOOtakes a parameterJOBS_FOOand callsBARwhich takesJOBS_BARit should just directly pass the argJOBS_BAR: {{.JOBS_FOO}}.A user wishing to set the default concurrency value can either set
TASK_EXTERNAL_TOOL_CONCURRENCYin their environment, or at the commandline:TASK_EXTERNAL_TOOL_CONCURRENCY=16 task ...Checklist
breaking change.
Validation performed
Manually tested with the boost test and prints.
Summary by CodeRabbit
New Features
Bug Fixes